Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check PR Author instead of Action Actor #137

Merged
merged 2 commits into from
Feb 21, 2022

Conversation

mwaddell
Copy link
Contributor

@mwaddell mwaddell commented Feb 7, 2022

closes #112

@mwaddell mwaddell requested a review from a team as a code owner February 7, 2022 17:25
@mwaddell mwaddell marked this pull request as draft February 12, 2022 02:36
@mwaddell mwaddell marked this pull request as ready for review February 12, 2022 02:36
@@ -6,7 +6,7 @@ permissions:
jobs:
dependabot:
runs-on: ubuntu-latest
if: ${{ github.actor == 'dependabot[bot]' }}
if: ${{ github.event.pull_request.user.login == 'dependabot[bot]' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ Thanks for updating the documentation as well

Copy link
Contributor

@brrygrdn brrygrdn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mwaddell! This change looks good to me.

@brrygrdn brrygrdn merged commit 4aac239 into dependabot:main Feb 21, 2022
@brrygrdn brrygrdn mentioned this pull request Feb 21, 2022
@xt0rted
Copy link

xt0rted commented Feb 25, 2022

I adjusted my workflow to take advantage of this change, but now it fails if I add commits to to the PR.

image

Typical reasons for this would be to add an entry to the changelog, fix linting rules, or as happened today fix an incorrect version update (a couple actions used v2 instead of v2.0.0).

What's the best way to work around this? Maybe a new output indicating if non-dependabot commits were found so you could skip subsequent steps?

@mwaddell
Copy link
Contributor Author

@brrygrdn What's the reason for failing the run if the PR contains commits after the original dependabot one? Is there a good reason not to delete https://github.com/dependabot/fetch-metadata/blob/main/src/dependabot/verified_commits.ts#L35:L38

@mwaddell
Copy link
Contributor Author

@brrygrdn What's the reason for failing the run if the PR contains commits after the original dependabot one? Is there a good reason not to delete https://github.com/dependabot/fetch-metadata/blob/main/src/dependabot/verified_commits.ts#L35:L38

@brrygrdn - I added #166 to remove those lines in order to address the issue noted by @xt0rted - let me know if there's a reason we can't do this.

@brrygrdn
Copy link
Contributor

brrygrdn commented Feb 26, 2022

That's a good question, it's something I've actually been blocked by myself this week. I originally added it as we were (defensively) trying to facilitate narrowest possible definition of a Dependabot PR, i.e.

  • Created by Dependabot
  • Exclusively containing code committed by Dependabot

I don't think any potential benefit or risk it mitigates really offsets the fact that an extremely common workflow like merging in the target branch is blocked. I'll have a chat with the team on Monday morning to get a second opinion, but I'm leaning towards removing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: Check PR Author Instead of Action Actor
3 participants